Fix Panorama segfault#5200
Conversation
| : IndexHNSWFlat(), cum_sums(), pano(0, 1, 1), num_panorama_levels(0) {} | ||
| : IndexHNSWFlat(), | ||
| cum_sums(), | ||
| pano(sizeof(float), 1, 1), |
There was a problem hiding this comment.
Needed so we don't have a division by zero when we do size_t n_real_levels = d / level_width_floats.
|
@mnorris11 Any advice on these OOMing tests? Is it something on my end? Edit: My new tests were too beefy |
|
@mnorris11 has imported this pull request. If you are a Meta employee, you can view this in D104764705. |
Thanks for the fix, can you explain this part more? |
|
@mnorris11 Yup! Consider the case where d = 960 and nlevels = 128. Ideally we would have However, the existing logic in the codebase is Thus, if the user requests a configuration like d = 960, nlevels = 128, we have to truncate nlevels to be something that can actually accommodate the available dims given a |
|
@mnorris11 merged this pull request in dd3003e. |
3 participants
In the current code you can segfault Panorama indexes by loading a dataset with 960 dims and setting
nlevelsto 128.In the case where
nlevelsdoes not evenly divided, Panorama currently calculateslevel_width = ceil(d / nlevels). We cannot change this behavior due to backwards compatibility. However, for our case of 960 dims and 128 levels, this would mean each level is 8 dims. Thus, the last 8 levels have 0 dims each. Since we calculateactual_level_widthusingsize_tparams, it overflows and produces 8 when it should produce 0, leading to out-of-bounds memory accesses.The fix is to simply truncate
nlevelsin thePanoramaobject when this would happen. A debug log is emitted to the user when this happens. In the aforementioned example, the user would seeWARNING truncating nlevels from 128 to 120.This PR also fixes a similar latent bug in
IndexFlatPanorama::reconstructwhere we would perform out-of-bounds memory accesses whennlevelsdoesn't evenly divided.Existing test cases are strengthened to ensure bugs are fixed and to avoid any future regressions.